Set the LLVM module flags for direct-access-external-data#158409
Set the LLVM module flags for direct-access-external-data#158409ilovepi wants to merge 1 commit into
Conversation
|
These commits modify compiler targets. |
|
r? @nnethercote rustbot has assigned @nnethercote. Use Why was this reviewer chosen?The reviewer was selected based on:
|
|
Note: I've included two commits, since I'm trying to solve a single problem, but its fine if these should be split up somehow. |
|
@nikic do you have thoughts here on how we can/should solve this on the llvm side (or if i've done something silly in terms of rustc)? |
This comment has been minimized.
This comment has been minimized.
b46d076 to
7f4494e
Compare
| extern "C" void LLVMRustSetModulePICLevel(LLVMModuleRef M) { | ||
| unwrap(M)->setPICLevel(PICLevel::Level::BigPIC); | ||
| Module *Mod = unwrap(M); | ||
| if (!Mod->getModuleFlag("PIC Level")) { | ||
| Mod->setPICLevel(PICLevel::Level::BigPIC); | ||
| } | ||
| } | ||
|
|
||
| extern "C" void LLVMRustSetModulePIELevel(LLVMModuleRef M) { | ||
| unwrap(M)->setPIELevel(PIELevel::Level::Large); | ||
| Module *Mod = unwrap(M); | ||
| if (!Mod->getModuleFlag("PIE Level")) { | ||
| Mod->setPIELevel(PIELevel::Level::Large); | ||
| } | ||
| } |
There was a problem hiding this comment.
Not totally convinced this is 100% the right approach. Alternatively, we could set the merge strategy and do it that way, but I figured its probably better to only replace if it doesn't exist. Happy to amend the strategy here.
| let llmod = module.module_llvm.llmod(); | ||
|
|
||
| let reloc_model = cgcx.relocation_model; | ||
| llvm::set_module_pic_and_pie_levels(llmod, reloc_model, &cgcx.crate_types); |
There was a problem hiding this comment.
I'm confused by why this is necessary. Aren't we just loading a previously serialized module here? Shouldn't the PIC/PIE flags have been set at the time the module was created?
There was a problem hiding this comment.
Yeah, I was surprised too. I believe the main contributor is that core & std libs end up w/o the PIE levels set at all, since they aren't being used in an executable. I can't recall if we have other examples or not, but its my vague recollection that we triggered this issue w/o use of core at all when codegen-units>1.
For our kernel specifically, this becomes an issue as we'd really like to use code from core. However, that would be a hard sell if we cannot prevent GOT indirection when accessing globals that would normally be safe to directly access (and have been accessed that way from C++ code for a very long time).
Do you think there's a better way to address this? Maybe somewhere in the lto.rs code?
There was a problem hiding this comment.
So the problem is that libcore/libstd are compiled as PIC but you want them to be PIE? And you're solving this by adding the PIE metadata during LTO? I don't think that's the right way to fix this.
Wouldn't the correct solution here to use build-std and enable PIE for libstd?
There was a problem hiding this comment.
They're being compiled into an executable, and our kernel builds core on demand as part of its build with all the same flags as the rest of the kernel. Under codegen-units>1 ThinLTO is always used, but the output isn't necessarily a PIE object, and this causes the kernel build to break due to the presence of GOT indirection (we have a rule preventing this in our linker script), even when we're not trying to use LTO if we don't setcodegen-units=1. This is all pretty surprising to us, as we didn't expect to have issues w/ LTO builds since they've been working forever in C++. FWIW, I'm pretty sure we tried setting all the various relocation-model flags on the core build w/o success, but we can try again.
I also think the test I added should just work w/ direct-access-external-data when compiling and linking against an rlib.
There was a problem hiding this comment.
Well, I'll be damned. That did solve the issue. So this does at least unblock me. Thanks for the suggestion. I had wrongly assumed that setting the platform defaults to position independent in the target spec would have amounted to the same thing, especially coupled with the fact that the Rust parts are only slotting into a minor part of the kernel build that's still mostly driven by clang.
I do still think the example in the test should be something that can work out of the box for platforms that default to PIE, though. That said, I'm guessing there may be a better way to spell that, such that we don't need about half of this patch. If you have thoughts about that I'm all ears.
In the meantime, unless there's an objection I'll pare this PR down to just adding the module flag and the minimal set of tests.
There was a problem hiding this comment.
I would expect setting it in the target spec to also work, as long as it's not explicitly overwritten on the command line. sess.relocation_model() looks like a pretty simple fallback to the target: self.opts.cg.relocation_model.unwrap_or(self.target.relocation_model).
In the meantime, unless there's an objection I'll pare this PR down to just adding the module flag and the minimal set of tests.
Sounds good to me.
There was a problem hiding this comment.
Done. I backed out the changes to the relocation model. I did leave some of the test infrastructure in place, since it is something I'd like to address somehow (I really do think that's behavior is a footgun), and if nothing else coverage of this bit of the compiler seems better with it. If you'd prefer I pare it down further or revise the wording of the description I'm happy to do so.
|
r? @nikic |
This comment has been minimized.
This comment has been minimized.
When compiling with ThinLTO, the missing module flag will cause the compiler to drop the `dso_local` from external data, leading to GOT slots being used. This breaks Rust usage in the Fuchsia kernel, and for embedded code that cannot use GOT indirection whenever ThinLTO is at play due to incorrect setting of the relocation model in the LLVM module when they are merged. Handling the relocation model will be done separately, as this module flag is a pre-requisite of for that change to affect direct-access-external-data. This patch ensures we emit the right module flag for the option. This matches clang's behavior. See https://github.com/llvm/llvm-project/blob/dbab3f71775f6edd11546385aeddb9d0b053a2b5/clang/lib/CodeGen/CodeGenModule.cpp#L1664
7f4494e to
1b5adcf
Compare
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
1b5adcf to
2155b48
Compare
Today, when using -Zdirect-access-external-data we've seen an issue when compiling code with ThinLTO or when codegen-units > 1 (due to the use of ThinLTO), that causes GOT accesses to occur in cases where they shouldn't. Unlike clang, rustc does not emit the direct-access-external-data module flag in the LLVM IR. When compiling with ThinLTO, the missing module flag will cause the compiler to drop the
dso_localfrom external data, leading to GOT slots being used. This breaks Rust usage in the Fuchsia kernel, and for embedded code that cannot use GOT indirection. The Clang example can be found here: https://github.com/llvm/llvm-project/blob/dbab3f71775f6edd11546385aeddb9d0b053a2b5/clang/lib/CodeGen/CodeGenModule.cpp#L1664There is potentially another separate issue due to the relocation model being set inconsistently, but that needs to be dealt with separately. For consumers that still experience this problem, an rlib that was built with relocation-model=pic, like
coreorstdcan be rebuilt with the-Crelocatiom-model=pieflag, which should result in the correct code generation behavior.